Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add close issue on move flag to board and add a cron to remove closed issues from board with that flag #28800

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

iminfinity
Copy link

add close issue on move flag to board which closes the moved issue
and add a cron to remove closed issues from board with that flag weekly

currently when a issue is added to project, it is never removed automatically, we dont want to spend time on removing closed issues from column

implements feature 1 of issue #27081


PR sponsored by Obmondo.com

image

Screen.Recording.-.Jan.12.2024.mp4

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 15, 2024
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jan 15, 2024
@lunny
Copy link
Member

lunny commented Jan 15, 2024

Looks like these are too hardcoded. Maybe we need a whole design before sending PRs.

Or maybe it can be implemented from actions if Gitea supported board events.

@iminfinity
Copy link
Author

iminfinity commented Jan 20, 2024

the lifecycle of an issue is complete after it is closed, at least in our org, and we really needed this feature, so closed issues doesnt clutter our kanbans, found this(hardcoded) as the fastest way to get this pr done and deployed

if Gitea supported board events, that means it doesnt do rn ?

should i try adding board events then to achieve this ? (or is it already in plan ?), i am happy to update code to make it mergeable so we can get this feature to everyone

@lunny
Copy link
Member

lunny commented Jan 22, 2024

should i try adding board events then to achieve this ? (or is it already in plan ?), i am happy to update code to make it mergeable so we can get this feature to everyone

I think yes, we need to add board events first.

@techknowlogick
Copy link
Member

I think yes, we need to add board events first.

I think we could land this PR before board events.

Default bool `xorm:"NOT NULL DEFAULT false"` // issues not assigned to a specific board will be assigned to this board
Sorting int8 `xorm:"NOT NULL DEFAULT 0"`
Color string `xorm:"VARCHAR(7)"`
CloseIssueOnMove bool `xorm:"DEFAULT false"`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as there are new fields added to a model struct, could you add a migration to add this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as there are new fields added to a model struct, could you add a migration to add this?

on a sidenote, regarding migration, i actually broke our prod with a migration(not this pr), it worked after i removed the migration, is there some doc about how to do migration correctly in gitea ?

@@ -747,5 +749,19 @@ func MoveIssues(ctx *context.Context) {
return
}

if board.CloseIssueOnMove {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to remove the if nesting, you could do something like

if !board.CloseIssueOnMove {
ctx.JSONOK()
return
}
//... board close logic

@@ -696,5 +699,19 @@ func MoveIssues(ctx *context.Context) {
return
}

if board.CloseIssueOnMove {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same as above

return errors.New("failed getting issue")
}

if err := ChangeStatus(ctx, issue, ctx.Doer, "", true); err != nil {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you could also remove if nesting here by checking if err is nil and moving the status change function call prior to the if, and then return in the if, and after the if, continue with the remaining logic.

)

// Cleanup removes closed issues from project board with close on move flag set to true
func CleanupProjectIssues(taskCtx context.Context, olderThan time.Duration) error {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

while this function would work with smaller instances, the resource consumption with larger ones can grow significantly with the several selects and no limits/pagination.

Could this logic be done with SQL itself, a join on projects where they have closeonmove, with issues that belong to a board that are closed? Then a separate statement to rm the issue from the project? Then in the rm statement(s) you could batch rm the rows from the DB?

@lunny
Copy link
Member

lunny commented Apr 3, 2024

@iminfinity Thank you for your contributions. But I think it's also hardcode. I think this PR should also be replaced by #30205 or changed after that PR merged as one event implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/translation size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants